Skip to content

Conversation

@andy-stark-redis
Copy link
Contributor

@andy-stark-redis andy-stark-redis commented Nov 15, 2024

DOC-4561

Let me know if this needs more emphasis or explanation, and if you have a better value for JWT_SECRET_KEY than yourKey, I'll add it in.

@andy-stark-redis andy-stark-redis requested review from a team and yaronp68 November 15, 2024 15:42
@andy-stark-redis andy-stark-redis self-assigned this Nov 15, 2024
@github-actions
Copy link
Contributor

Copy link
Collaborator

@dwdougherty dwdougherty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

```bash
tar -xvf rdi-k8s-<rdi-tag>.tar.gz
tar -xvf rdi-<rdi-tag>.tar.gz
Copy link
Collaborator

@dwdougherty dwdougherty Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like a tab character was used before "tar". I suggest replacing this with spaces. Here's an octal dump of the above snippet:

0000000    1   .           D   e   c   o   m   p   r   e   s   s       t
0000020    h   e       t   a   r       f   i   l   e   :  \n  \n        
0000040            `   `   `   b   a   s   h  \n  \t   t   a   r       -
0000060    x   v   f       r   d   i   -   <   r   d   i   -   t   a   g
0000100    >   .   t   a   r   .   g   z      \n                   `   `
0000120    `                                                            
0000121

In VS Code, the tar line does not line up with the indent of the line above.

Doesn't look like this affects the display, so consider it a quality of life change. :)

Copy link

@tahan42 tahan42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Note that if you want to use
[Redis Insight]({{< relref "/develop/tools/insight/rdi-connector" >}})
to connect to your RDI deployment and you are using TLS, make sure you
uncomment the `RDI_API_AUTH_ENABLED` line in the default `values.yaml`:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this setup is needed. TLS ingress is needed though if you're connecting with a RedisInsight outside the k8s cluster

@loriotpiroloriol
Copy link
Contributor

In values.yaml there are several instances where values apply to mTLS, but comments state TLS, specifically RDI_REDIS_CERT and RDI_REDIS_KEY in section rdiSysSecret and cert and key in section rdiDbSSLSecret.

@loriotpiroloriol
Copy link
Contributor

We should make it clear that JWT_SECRET_KEY should have 32 ASCII characters (256 bits, best practice).

@loriotpiroloriol
Copy link
Contributor

There is no mention about how to get redis-di installed to work alongside the Helm installation.

@loriotpiroloriol
Copy link
Contributor

We should make it clear which values need to be set in values.yaml as a minimum, i.e. RDI_REDIS_HOST, RDI_REDIS_PORT and RDI_REDIS_PASSWORD.

@andy-stark-redis andy-stark-redis merged commit 3c60f66 into main Nov 20, 2024
5 checks passed
@andy-stark-redis andy-stark-redis deleted the DOC-4561-rdi-helm-fixes branch November 20, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants